Note this is a significant change as we not longer expose this function. In discussion so far we've agreed that it basically should never have been pub, and I just made a mistake when doing so. An engine should call scan_data which mostly just proxies to this, but doesn't expose internal details to the engine.

Open to discussion though.

pub(crate) SGTM!

intentional/permanent change? Or just for debugging?

intentional, since this error occurs in more than one place

aside: I wonder if we should start adding some kind of "location code" as a (much) cheaper alternative to backtraces, that also stays stable as the code base evolves around it?

Yeah, that could work. I'm not too worried about perf for backtraces as they should only appear in error cases though

Agree on both nesting and rule of 30 here.

Also, this code is redundant:

let have_seen = self.check_and_record_seen(file_key);
if is_add && !have_seen {
    ... do stuff ...
}
Ok(is_add && !have_seen)

The early return would make very clear what's going on:

if !is_add || have_seen {
    return Ok(false);
}
... do stuff ...
Ok(true)

Related to #615:

Data skipping runs before this visitor, which means we can't use the partition values for data skipping in its current form.

How should we proceed? Even if we run a partition value extraction visitor before data skipping, that builds a hash map of parsed partition value literals (instead of embedding them in a struct expression), we still can't use the normal data skipping expression machinery. We'd almost need the row visitor itself to apply partition pruning, using a DefaultPredicateEvaluator that sits on top of the partition values map. The (big) downside of that approach is it won't reliably handle predicates that mix references to partition columns and normal columns, e.g. the following predicate would have no data skipping at all, because both predicate evaluators would reject the OR due to a missing leg:

WHERE partition_col1 = 10 OR value_col2 = 20

It would at least handle top-level AND gracefully, tho:

WHERE partition_col1 = 10 AND value_col2 = 20

(because each predicate evaluator would work with the subset of the AND it understands)

Even if we run a partition value extraction visitor before data skipping, that builds a hash map of parsed partition value literals (instead of embedding them in a struct expression), we still can't use the normal data skipping expression machinery.

Could you explain why we can't use the normal data skipping expression machinery? Current data skipping reads the stats field of add actions. I imagine we could use a visitor to extract the partition values along with the stats, then write back the stats field with updated values. Then data skipping proceeds as normal. idk if this is perhaps expensive, but I think it'll be important to be able to do data skipping on predicates with mixed references.

We definitely want the effect of data skipping, one way or another. I just meant that today's data skipping flow happens before the row visitor that could extract and parse partition values.

Either we need to add a second visitor that runs first and updates the stats column, or we apply partition skipping as a completely separate step (that could run before or after normal data skipping). Updating the stats column has several disadvantages:

1. Needs a separate visitor pass (runtime cost)
2. We don't currently have any API for updating an EngineData (we only have expression eval). We know we need to eventually add such capability, but we don't have it yet.
3. Stats-based pruning code isn't a great fit for partition values, because it wouldn't support nullcount based pruning and min/max based pruning is needlessly complex when always min=max for partition values.

That makes me wonder if we should apply partition pruning after stats-based pruning, as part of the existing row visitor that already filters out previously seen files:

• Parse partition values into a HashMap<ColumnName, Scalar>, which already has #[cfg(test)] impl ResolveColumnAsScalar in predicates/mod.rs (just need to remove the feature flag from it).
• Wrap a DefaultPredicateEvaluator around the partition values hashmap, and evaluate it.

Aha that makes sense. So move it till later to avoid complicating the existing data skipping and avoiding the runtime cost.

As for mixed references -- it will work for a majority of cases, because most partition predicates are simple top-level conjuncts, like this:

WHERE partition_col1 = 10 AND value_col2 = 20

The partition pruning code would handle the first conjunct (ignoring the second), and stats pruning code would handle the second conjunct (ignoring the first). This is actually how Delta-spark does it today.

Seems like being out was a great way for me to get this resolved :)

In seriousness though, that suggestion makes sense. We can let the existing flow prune via stats, and then just run the predicate evaluator over the extracted hashmap in the visitor, which can modify its already existing selection vector to prune files where the partition doesn't match.

wrt. to this PR, I think the code flow then still makes sense, and we can take partition pruning as a follow-up?

nit: we can perhaps flatten these imports.

aside: I wonder if we should start adding some kind of "location code" as a (much) cheaper alternative to backtraces, that also stays stable as the code base evolves around it?

aside: Looking at #624, I wonder if there's a (worthwhile) way to parse partition values only once per file action? But partition pruning and data fixup happen so far apart that I suspect it would be simpler (and maybe even cheaper) to parse a second time rather than try to build up and track a big side collection of parsed partition values.

It would perhaps be a different story if we had a clean way to convert partition values from string-string map to parsed struct using expressions, because then the partition values would be conveniently embedded in the log replay engine data. But I don't see that happening any time soon, given how much effort it would take to add map and string parsing expression support.

Yeah, this is a good point. Depending on how we merge things, we should consider looking at it when the second of this or #624 go in

Technically this isn't an expression (yet). Maybe better to call it partition_value (scalar), which then gets converted into a (literal) expression?

This seems like a dangerous change (because somebody trying to "optimize" the code might produce control flow that skips non-adds without checking them first). Now that you no longer need the have_seen multiple times, can we partially revert so it matches the code comment at L142 again?

Actually, we should probably update the comment to match the new code:

// Check both adds and removes (skipping already-seen), but only transform and return adds

Seems like a good place for let-else matching?

            let Expression::Struct(inner) = transform.unwrap().as_ref() else {
                panic!("Transform should always be a struct expr");
            };
            assert_eq!(...);
            
            let Expression::Column(ref name) = inner[0] else {
                panic!("Expected first expression to be a column");
            };
            assert_eq!(...);
            
            let Expression::Literal(ref scalar) = inner[1] else {
                panic!("Expected second expression to be a literal");
            };
            assert_eq!(...);

(less indentation => more readable)

I think it's using a Vec now?

nice catch. updated and updated the description

The comment doesn't reference column mapping? Should it?

Also, what other kind of transform might there be, besides "static" referenced here?

"Other transforms" means future things we may need to apply transforms for. So, variant decoding for example. If something needed variant decoding then the static_transform would not be None.

(idea for future) maybe worth introducing scan/transforms.rs for a transforms module?

also qq how does this actually become struct? just since you use that to colocate all the different exprs in the vec?

Yeah, not a bad idea on the module. I'll consider it when I handle merging this with #624.

A TransformExpr can either be a static "just select this col", or something that actually requires a transform, like "this is a partition col". In the log-replay code we iterate over this, turn the static "select cols" into Column expressions, and fill in the actual proper expressions for the others, which leaves a Vec<Expr> which we make a struct expr.

ah yep got it thanks!

is this desirable over self.get_static_transform() sort of API just so we can recycle/use it elsewhere in the future?

It would be, but to do that the tests for this functionality would always have to construct a snapshot, which is... hard at the moment. Perhaps when we switch to ResolvedTable we can simplify this

wonder if we we should consider something to communicate that this is some sort of unresolved/partial transform. might be useful to call out that the static part is already resolved as an actual Expr for transform but then partition requires resolution to actually deduce the expr (i realize this is comment above haha but took me a little to catch on so wondering if we could optimize naming)

Hopefully people read the comment :) I don't really want to bloat the name

mega nit but might be useful to log the index?